Skip to content

Conversation

@keeganirby
Copy link
Contributor

Problem

Addresses some feedback on past PRs

Solution

  • Removes CWL LIveTail limit preference. Instead pulls from existing limit preference. Updated description of limit preference to describe LiveTail line trimming behavior
  • Renames CWL Scheme variable to fit linting rules
  • Improves exception messaging when catching an on-stream failure

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@keeganirby keeganirby requested a review from a team as a code owner October 22, 2024 18:25
@keeganirby keeganirby changed the title Address PR Feedback refactor(cwl): Remove LiveTail specific limit preference, rename scheme, improve exception messaging Oct 22, 2024
} catch (err) {
throw new ToolkitError('Caught on-stream exception')
throw new ToolkitError(
`Encountered LiveTail on-stream failure for session ${session.uri}: ${(err as Error).message}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest not catching the error at all, just let it bubble up. please review our guidelines.

if you must catch and re-throw this, then it's usually a good idea to use ToolkitError.chain so that the original error is part of its chain.

Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after toolkiterror.chain() or bubble up

@justinmk3 justinmk3 merged commit 5ca4629 into aws:feature/cwltail Oct 22, 2024
22 of 24 checks passed
karanA-aws pushed a commit to karanA-aws/aws-toolkit-vscode that referenced this pull request Jan 17, 2025
…ws#5831

* Remove CWL LiveTail limit setting. Instead pulls from existing
  limit preference. Updated description of limit preference to describe
  LiveTail line trimming behavior
* Rename CWL Scheme variable to fit linting rules
* Bubble up exception instead of rethrowing it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants